Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove idle executors from widget #9177

Merged
merged 13 commits into from
Jun 18, 2024
Merged

remove idle executors from widget #9177

merged 13 commits into from
Jun 18, 2024

Conversation

mawinter69
Copy link
Contributor

@mawinter69 mawinter69 commented Apr 17, 2024 •
edited
Loading

showing idle executors brings no value. They only blow up the widget. Instead the busy/total number of executors are added behind the agent name when the agent is online.
The executor number is removed, to distinguish regular executors from tasks from asynchronous executions (flyweighttask) the latter get a special symbol.

Adds a new icon size class icon-xs so the symbols when agents are launching or are offline has better integration into the surrounding brackets and text).

The built-in agent is only shown when it has executors configured or when it is executing an async task that wants to show it's cell

When only a single agent is shown, the busy/total moves to the widget header.

When widget is collapsed, the shown text now distinguishes more different situations.

Before:

image

After:

Dashboard with several agents:

image

Dashboard with built-in no executors and one agent:

image
image
collapsed
image

Dashboard with built-in no executors and two agents:

collapsed:
image

Dashboard with built-in with 2 executors and one agent:

image
collapsed:
image

Dashboard with built-in with 2 executors and no agents:

image

collapsed:
image

Specific executor

image
collapsed:
image

No executors, agents or clouds

image

Symbols for offline and launching:

image

Testing done

manual testing

Proposed changelog entries

  • remove idle executors from widget

Proposed upgrade guidelines

N/A

Submitter checklist
Beta Give feedback

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist
Beta Give feedback

@mawinter69
remove idle executors from widget
110e465 
showing idle executors brings no value. They only blow up the widget.
Instead the busy/total number of executors are added behind the agent
name when the agent is online.
The executor name is kept intentionally to distinguish regular executors
from oneoffexecutors
@mawinter69 mawinter69 mentioned this pull request Apr 17, 2024
Draft
@daniel-beck
Copy link
Member

This is pretty nice 👍

One minor issue seems to be that without agents, the widget is empty now, as the header for Built-In Node only shows if you have agents. This may be confusing to (intermediate) users, it looks like nothing can build. (This may even be an opportunity to advertise https://www.jenkins.io/redirect/distributed-builds/ ?)

Another possible issue is that the executor number is shown and if you have several, those numbers appear and it's unclear what they're for or why someone should care. Are they exposed anywhere other than thread dumps? If not, would a symbol, or color, to distinguish between normal/heavyweight and lightweight executors work better?

(Also, JENKINS-72745 is extremely noticeable now, with a tooltip on the "0/2". Not new here, of course.)

@mawinter69
feedback
65ff1e3 
use an icon for oneoffexecutors (FlyweightTask), instead of using a
number for normal executors (which is also not really interesting).

When there are no agents and no builds running use the same text as when
collapsed to show that nothing is running.
@mawinter69
Copy link
Contributor Author

This is pretty nice 👍

One minor issue seems to be that without agents, the widget is empty now, as the header for Built-In Node only shows if you have agents. This may be confusing to (intermediate) users, it looks like nothing can build. (This may even be an opportunity to advertise https://www.jenkins.io/redirect/distributed-builds/ ?)

showing now the same text as when the widget is collapsed, still minor issue that when there are busy executors then the total configured is not shown.

Another possible issue is that the executor number is shown and if you have several, those numbers appear and it's unclear what they're for or why someone should care. Are they exposed anywhere other than thread dumps? If not, would a symbol, or color, to distinguish between normal/heavyweight and lightweight executors work better?

The executor number is also not really interesting so I changed it that only OneOffExecutors (Flyweighttask) use an icon (with tooltip)

(Also, JENKINS-72745 is extremely noticeable now, with a tooltip on the "0/2". Not new here, of course.)

That is probably JENKINS-72744

@mawinter69 mawinter69 marked this pull request as ready for review May 2, 2024 06:20
@NotMyFault NotMyFault requested a review from a team May 11, 2024 18:41
@NotMyFault NotMyFault added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label May 11, 2024
@github-actions github-actions bot added the unresolved-merge-conflict There is a merge conflict with the target branch. label May 17, 2024
@github-actions GitHub Actions
Copy link
Contributor

Please take a moment and address the merge conflicts of your pull request. Thanks!

@github-actions github-actions bot removed the unresolved-merge-conflict There is a merge conflict with the target branch. label May 18, 2024
timja
timja approved these changes Jun 16, 2024
View reviewed changes
Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM tested locally

@timja timja added the needs-security-review Awaiting review by a security team member label Jun 16, 2024
@timja timja requested review from a team June 16, 2024 19:02
@daniel-beck daniel-beck added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Jun 17, 2024
@timja
Copy link
Member

timja commented Jun 17, 2024

/label ready-for-merge

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jun 17, 2024
@timja timja merged commit 92ac1d8 into jenkinsci:master Jun 18, 2024
17 checks passed
@timja timja mentioned this pull request Jun 24, 2024
Closed
@timja
Copy link
Member

timja commented Jun 24, 2024

Likely caused jenkinsci/acceptance-test-harness#1603

@mawinter69 mawinter69 mentioned this pull request Jun 24, 2024
Merged
6 tasks
@MarkEWaite
Copy link
Contributor

I think that this causes a Jelly exception when the docker plugin attempts to edit a cloud definition. Refer to JENKINS-73554 for details.

Only visible in weekly releases, not in Jenkins LTS 2.452.3 or Jenkins LTS 2.462.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daniel-beck daniel-beck daniel-beck approved these changes

@timja timja timja approved these changes

Assignees
No one assigned
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted security-approved @jenkinsci/core-security-review reviewed this PR for security issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@mawinter69 @daniel-beck @timja @MarkEWaite @NotMyFault